@jlaur unfortunately I don't have any repeaters so I cannot test the new functionality, but I will at least check that existing functionality (normal shades) has not been broken :)

unfortunately I don't have any repeaters so I cannot test the new functionality

I would say fortunately. Not needing them is much desired. :-)

I will at least check that existing functionality (normal shades) has not been broken :)

Thanks. I have it running in production now, but would like to run it for some more time before marking as ready. Since it's a new thing type everything is quite isolated and existing functionality should not be affected much. But I already found an assumption in HDPowerViewHubHandler about all things being of type shade, so you never know.

A trivial issue, but one of your JUnit tests is issuing an unexpected logger message..

A trivial issue, but one of your JUnit tests is issuing an unexpected logger message..

I know, but didn't think it would be a problem since it's not considered a warning, but just two lines of console output? I'll have another look at this and see if I can mock logger somehow. Please don't let it block this PR as it was introduced by #11853 which is already merged. I'll provide a new PR for this issue and link it to #11852 once I have found a solution.

see if I can mock logger somehow

Perhaps one can set a lower logger level when running JUnit tests (e.g. log:set ERROR or even log:set OFF), or add an argument to the method being called (e.g. method(..., boolean loggingEnabled) -- although this latter is a bit clunky..)

EDIT: https://logging.apache.org/log4j/log4j-2.4/faq.html#set_logger_level_from_code

don't let it block this PR

Ok with me.

see if I can mock logger somehow

Perhaps one can set a lower logger level when running JUnit tests (e.g. log:set ERROR or even log:set OFF), or add an argument to the method being called (e.g. method(..., boolean loggingEnabled) -- although this latter is a bit clunky..)

I'm definitely against the latter. As for log level manipulration, I could imagine setting log level this way could introduce problems with parallel test runs, but not sure. Also it wouldn't be able to test if something was indeed logged.

Initially I tried to create some mocks based on LoggerFactory.getILoggerFactory() but failed. I looked into a few other approaches, and ended up with this static scoped one as being the cleanest and simplest for this scenario:

    @Test
    public void emptyListWhenNoSceneForScheduledEvent() {
        try (MockedStatic<LoggerFactory> staticMock = mockStatic(LoggerFactory.class)) {
            @Nullable
            final Logger logger = mock(Logger.class);
            staticMock.when(() -> LoggerFactory.getLogger(any(Class.class))).thenReturn(logger);

            builder = AutomationChannelBuilder.create(translationProvider, CHANNEL_GROUP_UID);
            ScheduledEvent scheduledEvent = createScheduledEventWithSceneCollection(
                    ScheduledEvents.SCHEDULED_EVENT_TYPE_SUNRISE);
            List<ScheduledEvent> scheduledEvents = new ArrayList<>(List.of(scheduledEvent));
            List<Channel> channels = builder.withScenes(scenes).withScheduledEvents(scheduledEvents).build();

            assertEquals(0, channels.size());
            Logger verifyLogger = verify(logger);
            assertNotNull(verifyLogger);
            if (verifyLogger != null) {
                verifyLogger.warn(any(String.class), any(), any());
            }
        }
    }

verify() can of course be adapted to needs, for this PoC it just verifies that any warning with two additional parameters is logged.

I was able to run it after modifying pom.xml for the binding:

Now wondering if mockito-inline could/should be added to:
https://github.com/openhab/openhab-core/blob/main/bom/test/pom.xml

which is referenced from here:
https://github.com/openhab/openhab-addons/blob/main/bom/test-index/pom.xml

@wborn - any opinion/advice here, i.e. if it would be okay to include mockito-inline instead of mockito-core, and if yes, in which POM? Eventually mockito-inline will be abandoned and we'll need to go back to mockito-core when integrated. See:

• https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#mockito-inline
• https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#39

IMHO you should certainly not add a mockito dependency to the binding pom..

Perhaps another solution is to have JUnit call an inner class for the test, that does not do any logging, but throws an exception instead; and then let the binding itself call an outer wrapper class that catches that exception and logs the warning.

PS notwithstanding the above, I can tell you that the jar for this build has been running on my operative system for 6 hours or so, and seems to be working as it should. (Although, actually I personally find it rather uncanny that when one moves the shades, the OH UI updates to the new position immediately — even though the shades have not actually achieved that position yet..)

IMHO you should certainly not add a mockito dependency to the binding pom..

We fully agree on this. It was a Proof-of-Concept, suggestion was to add it to the linked POM:
https://github.com/openhab/openhab-core/blob/main/bom/test/pom.xml

Perhaps another solution is to have JUnit call an inner class for the test, that does not do any logging, but throws an exception instead; and then let the binding itself call an outer wrapper class that catches that exception and logs the warning.

I'm not sure I understand what you mean by inner/outer classes, do you mean changing AutomationChannelBuilder itself? I rather think it should stay as a single class without any need to extract that private method to a new class just for sake of testing. Also would not throw/catch exceptions within a loop to handle such simple internal cases. Instead this could be changed to some try* pattern, though. The public method should be enough for testing that functionality within the private method also, we just need to mock the logger, and as I've shown, this is certainly possible.

PS notwithstanding the above, I can tell you that the jar for this build has been running on my operative system for 6 hours or so, and seems to be working as it should.

Thanks for giving it a shot!

(Although, actually I personally find it rather uncanny that when one moves the shades, the OH UI updates to the new position immediately — even though the shades have not actually achieved that position yet..)

Hmm, I think you introduced that behavior in #11698? It was slightly changed in #12002 by using the response directly instead of requesting positions again by sending a new request. I'm open for discussing this, it could even easily be reverted/changed/refined. However, please consider:

• We have no way of knowing how long it takes to move the shade, i.e. when it will reach its target position.
• The position will be updated in the hub immediately.

So with the new approach we introduced, we'll synchronize with the hub immediately. Previously (before your change) it would be synchronized at a random moment after moving the shade, i.e. upon next poll. So if next poll would be one second after moving the shade, it would be updated then. But it could also have to wait up to one minute.

not sure I understand what you mean by inner/outer classes

Oops. I meant to say “inner/outer method”. Sorry for the confusion.

introduced that behavior in #11698?

Yes indeed. But it was not my request, and came rather from the guy who opened the issue. As said, I personally find it uncanny … but I can live with it..

@andrewfg - I did a new commit, 271d027, today after some suspicious warnings in my logs. I still don't know what caused that, but I'm keeping an eye on it. Have you (ever) seen anything like that? Unfortunately I'm not keeping old logs, so can't check first occurrence. See last section in PR description for details.

any opinion/advice here, i.e. if it would be okay to include mockito-inline instead of mockito-core

I always stay away from experimental or incubating features because they can break with any upgrade. We've seen incubating mockito/junit functionality break before (openhab/org.openhab.binding.zwave#1689 (comment)).

@andrewfg - would you be able to review this PR before assigning it to maintainers for review? Asking because I now prepared a fix for #12071 but it will conflict, so will hold that back for a while.

@lolodomo - looking at recent hdpowerview PR review history, would this be something for you to have a look at? :-)

@openhab/add-ons-maintainers - anyone willing to review this PR? I have some additional work planned for another PR, but need this one merged first.

@openhab/add-ons-maintainers - anyone willing to review this PR? I have some additional work planned for another PR, but need this one merged first.

You could base your work on this PR's branch and add the "awaiting other PR" label for your new PR.

You could base your work on this PR's branch and add the "awaiting other PR" label for your new PR.

True. I'm just not a fan of such dependency chains as there could also be other things merged in the meantime. Or this one might not be merged at all. So when preparing small changes with big risk of conflicts, I tend to prefer waiting. But thanks for the hint!

The command looks good in general. Did you try to set the auto update policy to veto to prevent the necessity to reset the Channel value manually? See https://www.openhab.org/docs/developer/bindings/thing-xml.html#auto-update-policies

No, was unaware - thanks. Now after reading it, I think it will work better with current approach, since state will be kept for the duration of the blinking period. This looks kind of cool in the UI. :-) I don't think it's possible to set a timer using the policies?

There's no timer for update policies I'm aware of. Your approach is good to go.

@lolodomo - would it be possible for you to synchronize hdpowerview_da_properties with Crowdin as this was mistakenly included in this PR and a few others? Current situation is that Danish strings already exist in the repository, but they are not available in Crowdin.

would it be possible for you to synchronize hdpowerview_da_properties with Crowdin as this was mistakenly included in this PR and a few others? Current situation is that Danish strings already exist in the repository, but they are not available in Crowdin.

Is it ok to simply override all Crowdin translations by those in Github?

Is it ok to simply override all Crowdin translations by those in Github?

Yes, at least the Danish ("_da") ones which seems missing in Crowdin. I'm not sure if there are any other translations in Crowdin for hdpowerview not yet in the repository, but probably not? Can't figure out how to see all translations for a single bundle.

Translations uploaded in Crowdin. Only 29% of translations are completed.

Translations uploaded in Crowdin.

Thanks!

Do we have any guidelines or documentation for the processes involving Crowdin synchronization/workflows? By now I understand that default (English) properties are one-way synced from repository to Crowdin. Then translations are synced back to repository, and it creates some kind of trouble if they are going the other way - perhaps simply because there is no automation for that? But some details would be nice for the understanding. Also what it takes to assess if a Crowin PR is safe to merge. I can help write it if it doesn't exist, but first I would need some more knowledge. :)

Default (English) properties are one-way automatically synced from repository to Crowdin.
Non default properties are one-way automatically synced from Crowdin to repository when translations are fully completed in Crowdin.
Problems are introduced if non default translations are merged directly in repository (not coming from Crowdin). That is the reason why we have to reject PRs including such translations.
In Crowdin, like I did for the HDPowerview Danish translations, you have a command to upload translations from a file.
@wborn and @cweitkamp are the ones who have the best knowledge of all of that for the project. Please correct me if needed.